-
Notifications
You must be signed in to change notification settings - Fork 3.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
static builds link against musl #3343
Conversation
I'm in favor of pursuing this more if we care strongly about static builds. We will need to investigate potential changes in performance and talk to the |
Well, this is green. @mberhault: thoughts? |
no idea. I haven't encountered that particular bug. |
We definitely need to make sure we always use musl, not only in static On Mon, Dec 7, 2015, 17:42 marc notifications@github.com wrote:
|
@tschottdorf this patch does that, at least in the build docker container. @mberhault looks like golang/go#13492 is the ony issue on the books right now, which seems very unlikely to affect us. |
Ok, but that won't do it for linux users ( |
Unfortunately I'm not very familiar with musl and I'm not aware of anybody using RocksDB with musl. Maybe ask at https://www.facebook.com/groups/rocksdb.dev/? |
The discussion on the go/glibc bug makes it clear that statically linking libc is considered an exotic use case. So let's step back for a minute: why is it important for us to build fully-static (as opposed to mostly-static) binaries? The Go compiler itself is distributed as a binary which dynamically links libc. Why can't we do the same? |
I'm not an authority on this, but perhaps it allows us to run on any system, not just systems with the same libc of the same version or higher. The work was first done in a80fd74. |
Also not an authority on this, but from my point of view we did it purely for the deployment story. I'm also starting to think that distributing binaries which link only glibc dynamically is fine since that's what everyone else is doing (and since linking it statically is a bad idea with glibc), and since venturing out into unknown territory may not pay off. |
I've added a commit here that discontinues the use of static builds. If someone does try to build a static binary, we'll try to use musl (since glibc is known bad), but usual workflows (read: tests on AWS) will use dynamic linking. PTAL cc @mberhault |
rm -rf /tmp/* | ||
|
||
ENV CC=/usr/local/musl/bin/musl-gcc |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changes to this file also need to be backed out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, right. Though, only this line, not the "install musl" bit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not strictly necessary to remove the "install musl" bit, but it's wasteful to leave it in.
After some additional digging, I've backed out the "stop statically linking" commit - see the latest commit message for details. Also, I've removed the netgo build tag, since there's no benefit to using netgo now that we're using musl which provides proper static linking. |
This works around golang/go#13470 with the biggest hammer I could find. Another option is to go the route of dynamic linking, but this has unfortunate implications for our `c-*` dependencies' builds; for instance, in a dynamic build, c-rocksdb ends up linking against more than just glibc, greatly increasing the surface of libraries required to run cockroach. Having run such a build, the list of dependencies ends up being quite large: ``` $ ldd ./cockroach linux-vdso.so.1 (0x00007fff60bcc000) librt.so.1 => /lib/x86_64-linux-gnu/librt.so.1 (0x00007f60d0fc9000) libpthread.so.0 => /lib/x86_64-linux-gnu/libpthread.so.0 (0x00007f60d0dac000) libstdc++.so.6 => /usr/lib/x86_64-linux-gnu/libstdc++.so.6 (0x00007f60d0aa0000) libm.so.6 => /lib/x86_64-linux-gnu/libm.so.6 (0x00007f60d079f000) libgcc_s.so.1 => /lib/x86_64-linux-gnu/libgcc_s.so.1 (0x00007f60d0589000) libc.so.6 => /lib/x86_64-linux-gnu/libc.so.6 (0x00007f60d01df000) /lib64/ld-linux-x86-64.so.2 (0x00005620684d4000) ``` We could vigilantly produce static binaries upstream and lint agianst excessive dynamic dependencies, but the musl route is a shorter path to correctness.
Following this I was leaning towards leaving out |
@petermattis @bdarnell @mberhault would be good to have your opinions on this. The last commit message should provide useful context. |
Do we have any evidence that the list of "standard" dynamic libs that are in that commit message are problematic? |
My possibly misguided expectation is that at least |
LGTM (this unblocks some of the ec2 testing work, right?). I think for our final releases we probably want to link libc dynamically, though, so we should file a separate issue to figure out how to link everything but libc statically. |
Yeah, it avoids having weird workarounds to get the PGWire test running On Wed, Dec 9, 2015 at 8:00 PM Ben Darnell notifications@github.com wrote:
|
ec2 testing work is not currently blocked by this; the only test that reliably hits the original bug is TestPGWire, and that isn't being run in a static binary anywhere right now. |
I don't think we'll ever need to run on an environment as minimal as the busybox docker image (which is distinct from busybox itself). In addition to libc, we also need a zoneinfo database (and presumably enough of a support structure to keep it up to date, although I guess this could just be mounted in from the host machine). |
I'm inclined to merge this and file a follow-up issue as @bdarnell suggests. I'll do that after lunch tomorrow to allow time for objections. |
Still pro merging. But we should file a follow-up issue to figure out how to statically link everything but glibc. @bdarnell didn't find existing resources about that on the internet. Presumably this is an issue not only for |
static builds link against musl
This works around golang/go#13470.